Skip to content

Observe authenticated user to update profile #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jeffdgr8
Copy link
Contributor

  • UserProfileViewModel observes current user and updates profile state when new user logs in.
  • MainActivity injects UserProfileViewModel from Koin.
  • Composable views observeAsState() LiveData rather than accessing value directly so the views will recompose when LiveData values change.

@biozal
Copy link
Contributor

biozal commented Dec 15, 2022

Please review your code again - this introduces a regression bug where if I update a profile picture because Live Data doesn't observe appropriately on the main thread if I open the drawer, I see the older profile picture.

To replicate:
-Open Drawer
-Tap Update User Profile

  • Set image to new image
  • Tap Save
  • Now tap drawer icon
  • Review profile picture - it's the older profile picture, not the newer one.

Putting a breakpoint in will allow enough time for the thread to update and show the proper picture. Without a breakpoint, you end up with the older picture. I literally abandoned LiveData when I talked to some other engineers about this, and what everyone kind of agreed on was that LiveData has some issues with JPC. I moved over to mutableStateOf and it made life a lot easier.

If you update the PR with a workaround, let me know. This learning path is used a lot in our training materials and webinars, so I can't accept this in its current form because it makes the demo look like the database didn't update correctly with the saved image.

@biozal
Copy link
Contributor

biozal commented Dec 15, 2022

@jeffdgr8 FYI, my work around for this was the profileViewModel.updateUserProfileInfo call that happens in the openDrawer dev in the MainActivity. When it runs in the CorroutineScope, it allows the UserProfile widget to see the latest values. It was a workaround at the time to get this working and to ship. I'm open to a better long-term fix, but it hasn't been a super high priority.

@jeffdgr8
Copy link
Contributor Author

Thanks @biozal. I'll take a look and fix that. In the current state, the profile name, email, and team wasn't updating in the drawer at all after login. Definitely want to be sure it all works correctly though.

Personally, I've moved to using StateFlow instead of LiveData, as it's not tied to Android and works better in multiplatform. I used LiveData here because it was already being used elsewhere in the app though.

@biozal
Copy link
Contributor

biozal commented Dec 15, 2022

Thanks @biozal. I'll take a look and fix that. In the current state, the profile name, email, and team wasn't updating in the drawer at all after login. Definitely want to be sure it all works correctly though.

Personally, I've moved to using StateFlow instead of LiveData, as it's not tied to Android and works better in multiplatform. I used LiveData here because it was already being used elsewhere in the app though.

It should only update if the database has a value in it. If the database is empty, then the profile should only show what is set as default in the ViewModel. I added my function back in, and I think it fixes it even with LiveData, but I have to test it a bit more just to make sure. And yeah - when I have more time, I should probably look into a longer transition to StateFlow.

@biozal
Copy link
Contributor

biozal commented Dec 15, 2022

Thanks @biozal. I'll take a look and fix that. In the current state, the profile name, email, and team wasn't updating in the drawer at all after login. Definitely want to be sure it all works correctly though.

Personally, I've moved to using StateFlow instead of LiveData, as it's not tied to Android and works better in multiplatform. I used LiveData here because it was already being used elsewhere in the app though.

@jeffdgr8 The only place that should hopefully have LiveData in it hopefully is the Authentication stuff. That was just a leftover from a previous app that I used it from and didn't have time to convert it over to using the compose state management stuff. When I build tutorials, it's kind of like building Frankenstein's monster - I borrow code from other projects I've done if I know the code works well. The login is a really great example of some code I cut and pasted from another project to try and cut down on the time it would take to deliver this. I really should get better about refactoring this though because it doesn't match the rest of the code and looks out of place and that's not cool for clients reading it.

@jeffdgr8
Copy link
Contributor Author

@biozal I fixed the issue with profile changes syncing between UserProfileView and the nav drawer. There were a few changes:

  • First I rebased on Make DatabaseManager a Koin singleton #42, to avoid merge conflicts (it'll be easier to review after merging the other PR or compare isolated changes here).
  • Share UserProfileViewModel between UserProfileView and MainActivity so the nav drawer sees profile updates.
  • Ensure DatabaseManager.initializeDatabases() is called before setting the currentUser value in MockAuthenticationService, otherwise the user profile is fetched from the incorrect database and not found. (A real authentication service will work better as a suspend function as well, since it likely needs to make a network request.)
  • Introduce UserProfileRepository interface. UserProfileRepository was previously bound to KeyValueRepository for dependency injection, but KeyValueRepository is a generic interface, not specific to UserProfileRepository (although UserProfileRepository is the only KeyValueRepository in the app).
  • Make KeyValueRepository.get() return Map<String, Any?> as JSON documents may contain null or missing values. This avoids the need to check doc.contains() as well as explicitly cast to Any in UserProfileRepositoryDb.get().
  • Revert LiveData to use compose MutableState again in UserProfileViewModel, but use property delegate syntax to keep mutability private.
  • Ensure UserProfileViewModel properties are updated with new user, whether or not the new user profile contains the property (before the old user's property would be kept if new user didn't contain the property).

Avoid recompressing the same pic repeatedly
@jeffdgr8
Copy link
Contributor Author

One more bug fix added to avoid recompressing an unchanged user profile photo on every user profile save.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants